Added consistency checker by Ryangwaite · Pull Request #14402 · sonic-net/sonic-mgmt

Ryangwaite · 2024-09-03T11:49:09Z

Description of PR

Summary:
Added a new consistency checker utility that compares the values in the ASIC_DB with the values in the ASIC itself. It's initially integrated with test_upgrade_path.py.

It works by sending and receiving get request/responses to/from syncd via redis and pysairedis. To enable this, the following patches were made in dependent repositories:

Due to policies around backporting and merging changes to certain mainline branches, we can't merge the above changes in all cases. To support these, there are two flags --consistency_checker_libsairedis_url_template and --consistency_checker_python3_pysairedis_url_template where a templated URL may be provided to download libsairedis and python3-pysairedis debs that have been compiled for the test to be installed on the device.

This is an opt-in check and will only run if the --enable_consistency_checker has been provided.

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

Approach

What is the motivation for this PR?

There was a test gap discovered where the ASIC_DB and ASIC itself were inconsistent after an upgrade resulting in traffic being dropped.

How did you do it?

Created a python fixture called consistency_checker_provider that can be injected into any test and used to call the consistency_checker utility.

How did you verify/test it?

Tested on an internal A->B upgrade scenario.

Any platform specific information?

Currently only supports Arista x86_64-arista_7060_cx32s and x86_64-arista_7260cx3_64 platforms and versions 202305 and 202311. The ConsistencyCheckerProvider.is_consistency_check_supported will return True for a supported combination. Other platforms and versions may very well work they just haven't been tested yet. To try, simply ignore the result of is_consistency_check_supported and carry on with the test.

Supported testbed topology if it's a new test case?

Documentation

Will be added in a follow-up PR.

mssonicbld · 2024-09-03T11:49:46Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/upgrade_path/upgrade_helpers.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/common/fixtures/consistency_checker/consistency_checker.py:46:121: E501 line too long (128 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:57:121: E501 line too long (134 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:62:121: E501 line too long (144 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:83:121: E501 line too long (131 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:84:121: E501 line too long (220 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

tests/common/fixtures/consistency_checker/query-asic.py

mssonicbld · 2024-09-04T06:33:58Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/upgrade_path/upgrade_helpers.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/common/fixtures/consistency_checker/consistency_checker.py:46:121: E501 line too long (128 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:57:121: E501 line too long (134 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:62:121: E501 line too long (144 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:83:121: E501 line too long (131 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:84:121: E501 line too long (220 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

mssonicbld · 2024-09-05T05:53:05Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/common/fixtures/consistency_checker/query-asic.py Fixing tests/common/fixtures/consistency_checker/consistency_checker.py fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/common/fixtures/consistency_checker/consistency_checker.py:43:121: E501 line too long (128 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:54:121: E501 line too long (134 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:59:121: E501 line too long (144 > 120 characters) tests/common/fixtures/consistency_checker/consistency_checker.py:218:25: E117 over-indented ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

vaibhavhd

Non blocking comments added.

tests/common/fixtures/consistency_checker/query-asic.py

vaibhavhd · 2024-09-12T21:08:29Z

tests/common/fixtures/consistency_checker/consistency_checker.py

+
+        if self._python3_pysairedis_download_url is not None:
+            # Install python3-sairedis in syncd container
+            self._duthost.shell((f"docker exec {SYNCD_CONTAINER} bash -c "


DEB installation failures are not caught and not logged.

The kwarg module_ignore_errors kwarg is implicitly False here. So if the deb installation fails an error will be raised with the failing command. I don't think we should catch and log and continue.

tests/common/fixtures/consistency_checker/consistency_checker.py

vaibhavhd · 2024-09-12T21:25:14Z

tests/common/fixtures/consistency_checker/query-asic.py

+    finally:
+        uninitialize_sai_api()
+
+    print(json.dumps(results))


This will print to stdout. Why is logger not used?

vaibhavhd · 2024-09-12T21:29:45Z

tests/common/fixtures/consistency_checker/query-asic.py

+    logger.info("Initializing SAI API")
+    profileMap = dict()
+    profileMap[pysairedis.SAI_REDIS_KEY_ENABLE_CLIENT] = "true"
+    status = pysairedis.sai_api_initialize(0, profileMap)


Checking for pysairedis - is 0 for flags which should be zero as per sai spec?

https://github.com/opencomputeproject/SAI/blob/a0ae84d9366d35bfc50738c3b29e03387bf85c7c/inc/sai.h#L226

mssonicbld · 2024-09-13T06:35:20Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/common/fixtures/consistency_checker/query-asic/parser.py fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/common/fixtures/consistency_checker/consistency_checker.py:19:22: E222 multiple spaces after operator tests/common/fixtures/consistency_checker/query-asic/query-asic.py:155:25: E128 continuation line under-indented for visual indent tests/common/fixtures/consistency_checker/query-asic/query-asic.py:170:26: E128 continuation line under-indented for visual indent flake8...............................................(no files to check)Skipped ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

tests/common/fixtures/consistency_checker/consistency_checker.py

tests/upgrade_path/upgrade_helpers.py

tests/common/fixtures/consistency_checker/consistency_checker.py

tests/common/fixtures/consistency_checker/query-asic/query-asic.py

tests/common/fixtures/consistency_checker/consistency_checker.py

tests/upgrade_path/upgrade_helpers.py

…cker

StormLiangMS · 2024-09-19T00:45:32Z

hi @Ryangwaite could you confirm this is tested with 202311 and 202405 branch for cherry pick? Would like to avoid regression.

What is the motivation for this PR? There was a test gap discovered where the ASIC_DB and ASIC itself were inconsistent after an upgrade resulting in traffic being dropped. How did you do it? Created a python fixture called consistency_checker_provider that can be injected into any test and used to call the consistency_checker utility. How did you verify/test it? Tested on an internal A->B upgrade scenario. Any platform specific information? Currently only supports Arista x86_64-arista_7060_cx32s and x86_64-arista_7260cx3_64 platforms and versions 202305 and 202311. The ConsistencyCheckerProvider.is_consistency_check_supported will return True for a supported combination. Other platforms and versions may very well work they just haven't been tested yet. To try, simply ignore the result of is_consistency_check_supported and carry on with the test.

Added consistency checker

8e91f0c

Ryangwaite requested a review from prgeor as a code owner September 3, 2024 11:49

saiarcot895 reviewed Sep 3, 2024

View reviewed changes

tests/common/fixtures/consistency_checker/query-asic.py Outdated Show resolved Hide resolved

Handle parsing of MAC addresses

378d163

Tidied up code

7322b07

Ryangwaite added 2 commits September 5, 2024 06:27

Fix linting issues

a6b8db2

Added missing fixture injection in test_upgrade_path testss

8d95723

vaibhavhd approved these changes Sep 12, 2024

View reviewed changes

vaibhavhd added Request for 202311 branch Request for 202405 branch labels Sep 12, 2024

Addressed PR feedback

53f4be6

Addressed PR Feedback

ce433ae

Ryangwaite force-pushed the add-consistency-checker branch from 36b6e1c to ce433ae Compare September 13, 2024 06:42